Drop support of the old arch #27
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| moduleInfos.put( | ||
| RNBatchModuleImpl.NAME, | ||
| RNBatchModule.NAME, | ||
| new ReactModuleInfo( | ||
| RNBatchModuleImpl.NAME, | ||
| RNBatchModuleImpl.NAME, | ||
| false, // canOverrideExistingModule | ||
| false, // needsEagerInit | ||
| true, // hasConstants | ||
| false, // isCxxModule | ||
| BuildConfig.IS_NEW_ARCHITECTURE_ENABLED // isTurboModule | ||
| RNBatchModule.NAME, | ||
| RNBatchModule.NAME, | ||
| false, | ||
| false, | ||
| false, | ||
| true |
There was a problem hiding this comment.
The call to new ReactModuleInfo now passes only four boolean flags. The ReactModuleInfo constructor in React Native still expects five booleans (canOverrideExistingModule, needsEagerInit, hasConstants, isCxxModule, isTurboModule). Omitting one argument will make the Android library fail to compile once RNBatchPackage is built, blocking consumption of the package entirely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this constructor is deprecated now
| + (NSString *)moduleName | ||
| { | ||
| return @"RNBatchModule"; | ||
| } |
There was a problem hiding this comment.
The Objective‑C implementation no longer calls RCT_EXPORT_MODULE(), only defining +moduleName. The macro’s +load side effect registers the module with React Native; without it, the Turbo module will never be registered and JavaScript will receive null when requiring NativeRNBatchModule. This effectively makes the module unusable on iOS even though the rest of the implementation remains.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new ReactModuleInfo( | ||
| RNBatchModuleImpl.NAME, | ||
| RNBatchModuleImpl.NAME, | ||
| false, // canOverrideExistingModule | ||
| false, // needsEagerInit | ||
| true, // hasConstants | ||
| false, // isCxxModule | ||
| BuildConfig.IS_NEW_ARCHITECTURE_ENABLED // isTurboModule | ||
| RNBatchModule.NAME, | ||
| RNBatchModule.NAME, | ||
| false, | ||
| false, | ||
| false, | ||
| true |
There was a problem hiding this comment.
The new package code now calls new ReactModuleInfo(..., false, false, false, true) with only six boolean arguments. React Native 0.73 (the version still referenced in package.json) exposes only the seven‑argument constructor that includes the isTurboModule flag, so the project will no longer compile unless users upgrade to a much newer React Native. Please either restore the missing parameter or explicitly raise the minimum React Native requirement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
don't think so since it has been deprecated in 0.73, then removed later and re-introduced in 0.78
| * @return true if ready | ||
| */ | ||
| private boolean isModuleReady() { | ||
| return reactContext != null && reactContext.hasActiveCatalystInstance(); | ||
| return reactContext != null && reactContext.hasReactInstance(); |
There was a problem hiding this comment.
Event dispatching now relies on ReactApplicationContext.hasActiveReactInstance() and hasReactInstance(). These methods only exist in newer React Native releases; in 0.73 (still the declared dependency) the available methods are hasActiveCatalystInstance()/hasCatalystInstance(). Without updating the required React Native version this change results in a compilation failure for current consumers. Consider reverting to the older methods or documenting the higher minimum React Native version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
let's bump to 0.77, older react native version still maintained (hasReactInstance has been introduced in 0.75)
| /** | ||
| * Event Queue | ||
| * | ||
| * <p> |
There was a problem hiding this comment.
ca marche les tags html en javadoc
| .append(extension) | ||
| .append(fileExtension) | ||
| .toString(); | ||
| String fileName = FONTS_ASSET_PATH + fontFamilyName + extension + fileExtension; |
There was a problem hiding this comment.
pas de risque de formating/encoding sans passer par stringbuilder ?
There was a problem hiding this comment.
je pense pas, je t'avoue c'était un warning android studio je me suis pas posé de question
| promise.reject("InboxError", "No Context"); | ||
| return; | ||
| private BatchInboxNotificationContent findNotificationInList(List<BatchInboxNotificationContent> list, String identifier) { | ||
| for (BatchInboxNotificationContent notification : list) { |
There was a problem hiding this comment.
pas possible de fairre une sorte de return list.first(where: { $0.getNotificationIdentifier().equals(identifier) }) ?
There was a problem hiding this comment.
si on était minSdk 24 on pourrait utiliser les streams mais on l'est pas .
Removed support for the old React Native architecture (Native Module). The plugin is now a pure Turbo Module and requires the new architecture enabled.